Handle floating promises #3492

siltomato · 2025-10-03T23:07:03Z

This PR fixes all lint no-floating-promises warnings. All files used the void methodology to handle the floating promises (maintaining existing fire-and-forget behavior) except for the two instances described below.

In the lynx-insight-panel.component.ts onLeafNodeClick() function, the promise chain is changed to async/await. This is more of a readability change than functional, as, in both cases, the nav completes before the call to update display state.
In the realtime-doc.ts delete() function:

async delete(): Promise<void> {
  await this.adapter.delete();
  await this.updateOfflineData();
  await this.realtimeService.onLocalDocUpdate(this);
}

await this.adapter.delete() was chosen instead of void this.adapter.delete() with the idea to maintain data consistency in the case that the adapter.delete() call fails. Please review if this is the correct approach.

This change is

codecov · 2025-10-03T23:15:34Z

Codecov Report

❌ Patch coverage is 77.11864% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.30%. Comparing base (eea0a15) to head (ef08444).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...Forge.Scripture/ClientApp/src/app/app.component.ts	57.14%	6 Missing ⚠️
...ientApp/src/xforge-common/user-projects.service.ts	0.00%	4 Missing ⚠️
...slate/draft-generation/draft-generation.service.ts	62.50%	3 Missing ⚠️
...ntApp/src/app/translate/editor/editor.component.ts	84.21%	3 Missing ⚠️
...p/src/app/machine-api/remote-translation-engine.ts	33.33%	2 Missing ⚠️
...e/ClientApp/src/app/shared/project-router.guard.ts	0.00%	2 Missing ⚠️
...anslate/biblical-terms/biblical-terms.component.ts	71.42%	0 Missing and 2 partials ⚠️
...eneration/draft-sources/draft-sources.component.ts	0.00%	2 Missing ⚠️
...nx-insights-panel/lynx-insights-panel.component.ts	50.00%	2 Missing ⚠️
...pp/src/xforge-common/exception-handling.service.ts	50.00%	2 Missing ⚠️
... and 25 more

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3492   +/-   ##
=======================================
  Coverage   82.30%   82.30%           
=======================================
  Files         613      613           
  Lines       36877    36879    +2     
  Branches     6046     6046           
=======================================
+ Hits        30352    30354    +2     
  Misses       5629     5629           
  Partials      896      896

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Nateowami · 2025-10-04T02:58:18Z

Thanks for tackling this @siltomato. One major concern I have is that it's going to create a slew of merge conflicts with #3199, which is already big, difficult, and affects async/await in a lot of places. Maybe we should merge the two instances you found that you think should be awaited, and then have the rest of them updated after #3199 to reduce conflicts (or at least to simplify the conflicts, since adding void is a lot simpler than the other PR's changes)?

siltomato

I tested merging this branch with #3199, and there were merge conflicts in just 5 files that were pretty straightforward to fix. But just for peace of mind, there's no harm in just waiting to merge this until that branch is merged.

Reviewable status: 0 of 70 files reviewed, all discussions resolved

Nateowami · 2025-10-08T21:26:01Z

@siltomato 5 straightforward fixes is very manageable. Let's proceed.

Copilot

Pull Request Overview

This PR addresses lint no-floating-promises warnings throughout the codebase by adding void to promises that are intentionally fire-and-forget. Two instances in the editor components use async/await for improved readability, and one instance in realtime-doc.ts uses await to maintain data consistency during deletion operations.

Key Changes

Added void prefix to floating promises to maintain existing fire-and-forget behavior while satisfying linting requirements
Converted onLeafNodeClick() in lynx insights panel to async/await pattern for better readability
Changed delete() method in realtime-doc.ts to await adapter deletion for data consistency

Reviewed Changes

Copilot reviewed 69 out of 69 changed files in this pull request and generated 2 comments.

File	Description
`src/xforge-common/models/realtime-doc.ts`	Changed delete() method to await adapter deletion for consistency
`src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts`	Converted onLeafNodeClick() to async/await pattern
Multiple service and component files	Added void prefix to floating promises across the application

Copilot · 2025-10-08T21:26:30Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts

  async delete(): Promise<void> {
-    this.adapter.delete();
+    await this.adapter.delete();
    await this.updateOfflineData();
    await this.realtimeService.onLocalDocUpdate(this);
  }


The change to await this.adapter.delete() is correct for maintaining data consistency. However, consider adding error handling to ensure the subsequent operations don't execute if the adapter deletion fails, which could leave the document in an inconsistent state.

In the other pull request which deals with realtime doc cleanup, the order in which the disposal methods are called turns out to be very complicated and very important. I would suggest that we leave this line unchanged and fix it in the other pull request in the interest of simplicity. Basically, the situation is just too complicated for me to mentally untangle and be certain that this change is going to work with the changes in the other PR.

Ok, changed await to void.

I suggest we leave this line unchanged. The change might be correct, and the existing behavior could even be wrong. I'd rather have a lint warning reminding us to deal with it. Marking it void is essentially saying "a developer looked at this and conclusively determined that we don't want to await this", which is definitely not correct.

Good point. Changed.

.../src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts

siltomato

Reviewable status: 0 of 70 files reviewed, all discussions resolved (waiting on @Nateowami)

.../src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts

siltomato · 2025-10-08T21:43:49Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts

  async delete(): Promise<void> {
-    this.adapter.delete();
+    await this.adapter.delete();
    await this.updateOfflineData();
    await this.realtimeService.onLocalDocUpdate(this);
  }


Ok, changed await to void.

Nateowami

@Nateowami dismissed @copilot from a discussion.
Reviewable status: 0 of 70 files reviewed, all discussions resolved

Nateowami · 2025-10-08T22:27:57Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts

  async delete(): Promise<void> {
-    this.adapter.delete();
+    await this.adapter.delete();
    await this.updateOfflineData();
    await this.realtimeService.onLocalDocUpdate(this);
  }


I suggest we leave this line unchanged. The change might be correct, and the existing behavior could even be wrong. I'd rather have a lint warning reminding us to deal with it. Marking it void is essentially saying "a developer looked at this and conclusively determined that we don't want to await this", which is definitely not correct.

siltomato

Reviewable status: 0 of 70 files reviewed, all discussions resolved (waiting on @Nateowami)

siltomato · 2025-10-09T00:20:50Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts

  async delete(): Promise<void> {
-    this.adapter.delete();
+    await this.adapter.delete();
    await this.updateOfflineData();
    await this.realtimeService.onLocalDocUpdate(this);
  }


Good point. Changed.

RaymondLuong3

Just a few things to clean up. Otherwise, thanks for the improvement.

@RaymondLuong3 reviewed 62 of 70 files at r1, 7 of 7 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @siltomato)

src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.ts line 130 at r4 (raw file):

      void this.router.navigate(['projects', projectId]);
    } catch {
      void this.noticeService.show(this.i18n.translateStatic('my_projects.failed_to_join_project'));

No longer necessary if show() and showError() are synchronous.

src/SIL.XForge.Scripture/ClientApp/src/app/checking/question-dialog/question-dialog.service.ts line 47 at r4 (raw file):

    }
    if (!(await this.canCreateAndEditQuestions(config.projectId))) {
      void this.noticeService.show(this.transloco.translate('question_dialog.add_question_denied'));

This would not need to be voided if show is no longer asynchronous.

Code quote:

 void this.noticeService.show(this.transloco.translate('question_dialog.add_question_denied'));

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts line 554 at r4 (raw file):

    } else {
      if (!this.onlineStatusService.isOnline) {
        void this.noticeService.show(this.i18n.translateStatic('draft_generation.offline_message'));

No longer necessary if show is synchronous

Code quote:

void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.ts line 235 at r4 (raw file):

    if (error.code === DOMException.NOT_FOUND_ERR) {
      void this.noticeService.show(this.i18n.translateStatic('checking_audio_recorder.mic_not_found'));

No longer necessary if show() is synchronous.

Code quote:

void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 221 at r4 (raw file):

    } catch (err) {
      console.error('Error occurred while saving draft format', err);
      void this.noticeService.showError(this.i18n.translateStatic('draft_usfm_format.failed_to_save'));

No longer necessary if showError() is synchronous

Code quote:

oid this.noticeService.showError(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts line 304 at r4 (raw file):

      this.userAppliedDraft = true;
    } catch (err) {
      void this.noticeService.showError(this.i18n.translateStatic('editor_draft_tab.error_applying_draft'));

No longer necessary if show is synchronous

Code quote:

void this.noticeService.showError(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1806 at r4 (raw file):

      this.translator = undefined;
      if (this.translationSuggestionsEnabled) {
        void this.noticeService.show(this.i18n.translateStatic('editor.verse_too_long_for_suggestions'));

No longer necessary if show() is synchronous.

Code quote:

void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.ts line 457 at r4 (raw file):

      });
    } else if (likeAnswerResponse === LikeAnswerResponse.DeniedOwnAnswer) {
      void this.noticeService.show(this.i18n.translateStatic('checking_answers.cannot_like_own_answer'));

this should no longer need to be voided. And below.

Code quote:

  void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 275 at r4 (raw file):

      this._userRows = userRows.concat(invitees);
    } catch {
      void this.noticeService.show(this.i18n.translateStatic('collaborators.problem_loading_invited_users'));

No longer necessary if show() is synchronous.

Code quote:

void this.noticeService.show(t

src/SIL.XForge.Scripture/ClientApp/src/app/machine-api/remote-translation-engine.ts line 108 at r4 (raw file):

        );
      } else {
        void this.noticeService.showError(this.i18n.translateStatic('error_messages.failed_to_retrieve_suggestions'));

No longer necessary if showError() is synchronous.

Code quote:

void this.noticeService.showError(

src/SIL.XForge.Scripture/ClientApp/src/app/checking/import-questions-dialog/import-questions-dialog.component.ts line 503 at r4 (raw file):

      data
    ) as MatDialogRef<ImportQuestionsConfirmationDialogComponent, ImportQuestionsConfirmationDialogResult>;
    const result: ImportQuestionsConfirmationDialogResult | undefined = await lastValueFrom(dialogRef.afterClosed());

It looks like this is the only place where an await is used in this method. We could just subscribe to the afterClosed() observable and run the logic in the listener and this method wouldn't need to be asynchronous.

Code quote:

await lastValueFrom(dialogRef.afterClosed());

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1291 at r4 (raw file):

    const answerIndex = this.getAnswerIndex(answer);
    if (answerIndex >= 0) {
      void activeQuestionDoc

This might be one of those places where we would benefit from awaiting the promise. Assuming that refresh summary depends on the number of answers in the question.

Code quote:

      void activeQuestionDoc

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync.component.ts line 111 at r4 (raw file):

      ) {
        if (this.projectDoc.data.sync.lastSyncSuccessful) {
          void this.noticeService.show(

No longer necessary if show() is synchronous.

Code quote:

 void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 216 at r4 (raw file):

  }

  async expireToken(): Promise<void> {

This can be made a synchronous method.

Code quote:

  async expireToken(): Promise<void> {

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/exception-handling.service.ts line 188 at r4 (raw file):

      ngZone.run(
        () =>
          void noticeService.showError(this.i18n.translateStatic('exception_handling_service.network_request_failed'))

No longer necessary if show() is synchronous. And 1 instance below.

Code quote:

 void noticeService.showError(t

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts line 50 at r4 (raw file):

  }

  async show(message: string): Promise<void> {

Same as below.

Code quote:

  async show(message: string): Promise<void> {

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts line 54 at r4 (raw file):

  }

  async showError(

Same as below

Code quote:

  async showError(

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts line 62 at r4 (raw file):

  }

  private async showSnackBar(

You can remove the async from this function and update the Promise return value to be void.

Code quote:

private async showSnackBar(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-history/history-chooser/history-chooser.component.ts line 145 at r4 (raw file):

  async revertToSnapshot(): Promise<void> {
    if (!this.onlineStatusService.isOnline) {
      void this.noticeService.show(this.i18n.translateStatic('history_chooser.please_connect'));

No longer necessary if show and showError is synchronous. And 3 instances below.

Code quote:

void this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/training-progress/training-progress.component.ts line 144 at r4 (raw file):

            // training completed successfully
            if (this.trainingProgressClosed) {
              void this.noticeService.show(

No longer necessary if show() is synchronous.

Code quote:

oid this.noticeService.show(

src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 189 at r4 (raw file):

    }

    void this.noticeService.show(message);

No longer necessary if show is synchronous.

Code quote:

 void this.noticeService.show(message);

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/user.service.ts line 127 at r4 (raw file):

        await this.updateAvatarFromDisplayName();
      } catch {
        void this.noticeService.showError(translate('error_messages.failed_to_update_avatar'));

No longer necessary if showError is synchronous.

Code quote:

void this.noticeService.showError(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 78 at r4 (raw file):

        }

        void this.noticeService.showError(this.i18n.translateStatic('draft_generation.temporarily_unavailable'));

No longer necessary if showError() is synchronous. And 6 instances below.

Code quote:

void this.noticeService.showError(

siltomato added the testing not required label Oct 3, 2025

siltomato marked this pull request as ready for review October 3, 2025 23:10

siltomato force-pushed the task/handle-floating-promises branch from c001568 to 45423d5 Compare October 8, 2025 20:55

siltomato commented Oct 8, 2025

View reviewed changes

Nateowami requested a review from Copilot October 8, 2025 21:26

Copilot AI reviewed Oct 8, 2025

View reviewed changes

siltomato commented Oct 8, 2025

View reviewed changes

Nateowami reviewed Oct 8, 2025

View reviewed changes

siltomato added 3 commits October 8, 2025 20:19

handle lynx floating promises

4e2ed4e

handle all floating promises

fab9852

remove 'await' for realtime-doc delete

ef08444

siltomato force-pushed the task/handle-floating-promises branch from 0bb903a to ef08444 Compare October 9, 2025 00:19

siltomato commented Oct 9, 2025

View reviewed changes

RaymondLuong3 self-assigned this Oct 22, 2025

RaymondLuong3 requested changes Oct 22, 2025

View reviewed changes

Uh oh!

Handle floating promises #3492

Are you sure you want to change the base?

Handle floating promises #3492

Conversation

siltomato commented Oct 3, 2025 • edited by irahopkinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Nateowami commented Oct 4, 2025

Uh oh!

siltomato left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami commented Oct 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Nateowami Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

siltomato Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Nateowami Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

siltomato Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siltomato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siltomato Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

siltomato left a comment

Choose a reason for hiding this comment

Uh oh!

siltomato Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

siltomato commented Oct 3, 2025 •

edited by irahopkinson

Loading

codecov bot commented Oct 3, 2025 •

edited

Loading